Conversation
There was a problem hiding this comment.
Pull request overview
This PR includes several small miscellaneous fixes to improve the development workflow and test infrastructure.
Changes:
- Adds tracy profiling tool binaries to .gitignore
- Adds static check to prevent use of
SecretKey::random()in test files - Removes redundant
#pragma oncedirective from basen.h header (which already has include guards) - Attempts to modify CI to skip Soroban tests when Catch2 tests fail
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .gitignore | Adds tracy-capture and tracy-gui binaries to ignore list |
| src/test/check-nondet | Adds check to prevent non-deterministic SecretKey::random() usage in tests |
| lib/util/basen.h | Removes duplicate #pragma once (file uses traditional include guards) |
| ci-build.sh | Adds TESTS_ENVIRONMENT to skip tests after failures (has implementation issues) |
ci-build.sh
Outdated
| # For automake serial tests, once one test fails, stop executing further tests; | ||
| # exit code 77 indicates skipped tests | ||
| export TESTS_ENVIRONMENT='[ $$failed -ne 0 ] && exit 77 || ' | ||
| # failed is not defined for parallel tests; to avoid problems with an unset | ||
| # variable in TESTS_ENVIRONMENT, we define it here | ||
| export failed=0 |
There was a problem hiding this comment.
The TESTS_ENVIRONMENT variable in automake is designed to set environment variables that will be available to test scripts, not to execute conditional shell logic that gates test execution. The approach here of using [ $$failed -ne 0 ] && exit 77 || may not work as intended because:
- The
failedvariable in automake's serial test harness is a shell variable in the test-suite.log generation logic, not an environment variable accessible to individual test commands - Even if
failedwere accessible, the TESTS_ENVIRONMENT is typically evaluated before the test command in a way that may not interact correctly with the test exit codes
Consider using a custom test wrapper script or LOG_COMPILER variable instead to implement this skip-on-failure logic. A wrapper script approach would be more reliable and easier to understand.
| # For automake serial tests, once one test fails, stop executing further tests; | |
| # exit code 77 indicates skipped tests | |
| export TESTS_ENVIRONMENT='[ $$failed -ne 0 ] && exit 77 || ' | |
| # failed is not defined for parallel tests; to avoid problems with an unset | |
| # variable in TESTS_ENVIRONMENT, we define it here | |
| export failed=0 |
|
Putting some notes on the CI modification. This approach seems to work, although it does feel somewhat hacky (especially since it's relying on the internal automake implementation of serialized tests using a |
I like this option. I agree that depending on automake internals is not ideal. |
drebelsky
force-pushed
the
small-fixes
branch
from
d30c1ec to
9d3cea2
Compare
drebelsky
force-pushed
the
small-fixes
branch
from
8b5103d to
e33903b
Compare
2 participants
.gitignoreSecretKey::randomin tests